Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement function (0x2B / 0x0E) Read Device Identification. #649

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jcarrano
Copy link

@jcarrano jcarrano commented Aug 10, 2022

Description

This implements the function (0x2B / 0x0E) as defined in section 6.21 of the Modbus Application Protocol v1.1b.

I'm aware of PR #443 , but that one uses the method "04" which will be less supported than "01" which is used here - this one supports all four methods.

Testing

I have tested this on a Modbus/TCP device. I will test on a RTU device on the coming days. I'm not sure how the testing works in general for this library.

The function is defined in section 6.21 of the Modbus Application Protocol v1.1b.
There are many ways to read the objects. The one used here is code 01, which seems
to be the most widely supported one.

It might be necessary to make many requests in order to receive all the data. This
is all handled internally.
@cla-bot
Copy link

cla-bot bot commented Aug 10, 2022

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

@jcarrano
Copy link
Author

Shall I fill in the CLA before this gets reviewed?

@karlp
Copy link
Contributor

karlp commented Aug 11, 2022

IMO this is too narrow support, you've hardcoded to function 1 because you believe it's more widely used. I'd like to see better underlaying support for the primitives, so that they can be composed by users, rather than just a single "does everything, but only via this single style of operation" function. Do you have hardware that implements stream access to basic, but not specific object access? I'm also not entirely sure that the nicest API is to continue re-reading as long as the server keeps indicating that it has more data, you're can block ~indefinitely now, if the server just keeps spewing data.

And yes, you should absolutely fill in CLA's before anyone with any authority looks at it. Why should anyone spend time reviewing code if someone then refuses to sign the CLA?

@jcarrano
Copy link
Author

@karlp First, WRT to the CLA, it is not problem to sign it, I just did not want to bother my boss before I knew there was interest in merging that. I will get you the CLA ASAP.

I have a Siemens PAC2200 electric meter which only supports mode 01, here is the response:

./tests/.libs/dev-id-test-client 0 50 50 50
Connecting to 127.0.0.1:1502
[00][00][00][00][00][05][FF][2B][0E][01][00]
Waiting for a confirmation...
<00><00><00><00><00><49><FF><2B><0E><01><01><00><00><03><00><0A><53><69><65><6D><65><6E><73><20><41><47><01><07><50><41><43><32><32><30><30><02><2A><46><57><20><56><33><2E><30><2E><31><31><2E><30><5F><31><2E><31><2E><30><2E><31><20><2F><20><42><4C><20><56><33><2E><30><2E><30><2E><30><5F><31><2E><31><2E><30><2E><31>
mf: 0, oid: 0, to_add: 3
Requested: 3, Read: 3
Obj ID	Length	Trunc?	Value
00	 10	 	Siemens AG	[53][69][65][6D][65][6E][73][20][41][47]
01	  7	 	PAC2200	[50][41][43][32][32][30][30]
02	 42	 	FW V3.0.11.0_1.1.0.1 / BL V3.0.0.0_1.1.0.1	[46][57][20][56][33][2E][30][2E][31][31][2E][30][5F][31][2E][31][2E][30][2E][31][20][2F][20][42][4C][20][56][33][2E][30][2E][30][2E][30][5F][31][2E][31][2E][30][2E][31]

The relevant part is <2B><0E><01><01>

The modbus specification seems to indicate that there is a sort of "hierarchy" of modes:

If the server device is asked for a description level ( readDevice Code )higher that its
conformity level , It must respond in accordance with its actual conformity level.

In any case, the structure of the response is the same for all mode, so it would be quite easy to add a parameter to the function to select the mode.

You are right that a buggy device could cause an infinite loop and that is bad. It could be solved by exiting the loop if zero objects are returned, but I think it is better to just remove the loop.

What do you think about the following prototype?

int modbus_read_device_id(modbus_t *ctx, int access_type, int object_id, int max_objects,
                                             uint8_t *obj_values[], int obj_lengths[], int *more_follows);

The internal loop would be eliminated, and if there is more data, the "more follows" value would be returned. Should I add an additional output parameter to retrieve the "conformity level" (i.e. which read modes are supported)?

Another option would be to have two functions, one for stream access, and another one for individual access. The "more_follows" parameter is not required for the latter and the obj_values and obj_lengths don't have to be arrays there.

Also, retrieve the internal parameters "more follows", "next object" and
the conformity level.

The value of the next object is clipped, otherwise, the users would have to be
very careful when writing loops since a buggy device could send them into an
infinite loop.
@cla-bot
Copy link

cla-bot bot commented Aug 11, 2022

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please fill https://forms.gle/5635zjphDo5JEJQSA to get added. Your document will be manually checked by the maintainer. Be patient...

@jcarrano
Copy link
Author

I signed the CLA and also made changes to the code:

  • All read methods are supported
  • The user is not responsible for looping in order to retrieve all the values
  • The conformity level is reported back to the user
  • A few cleanups.

I'm still not sure whether the "parallel arrays" interface is a good thing or wether the function should take in an array of structures of the form:

struct object {
   int id;
   int obj_size;
   int buffer_size;
   uint8_t *buffer;
};

or maybe instead of buffer_size call it actual_size and have the library place the minimum there (i.e. to spare the user from having to compute that again).

@jcarrano
Copy link
Author

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Aug 23, 2022

The cla-bot has been summoned, and re-checked this pull request!

@cla-bot cla-bot bot added the cla-signed label Aug 23, 2022
@jcarrano
Copy link
Author

Hi, any update on this? Is there anything that needs to be changed?

AFAIU there is no way to implement this using send_raw_request and receive_confirmation because of the way that the packet length is computed. Also, I have devices that only expose some data through the Object address space so that means that the data is not accessible using "vanilla" libmodbus.

@DaAwesomeP
Copy link

Hello, has this been tested on a RTU device?

@DaAwesomeP
Copy link

Similar to #474, #261. Seems like another feature with lots of open pulls but no merges.

@jcarrano
Copy link
Author

@DaAwesomeP yes, it is tested with RTU and TCP, but with "basic" support level, though that should not be an issue because the response format is the same in all cases.

#261 is about the server side support.
#474 is somehow similar. Interestingly, one of the criticisms was that the client had to handle "more-follows", "next-id" etc, while in this PR, the opposite issue was raised:

I'd like to see better underlaying support for the primitives, so that they can be composed by users

so I changed the implementation to not handle "more-follows", "next-id" automatically. This resulted in an interface that requires the user too loop to get all results.

I'd like to make a few changes:

@jcarrano
Copy link
Author

jcarrano commented Dec 5, 2022

@DaAwesomeP , @karlp I'd very much like to have this feature merged in the official repo, instead of keeping it as a private patch. I'm 100% willing to make any enhancements or modifications that the maintainers request. What is blocking this feature? Is it lack of interest, or lack of hardware to test?

@jovere
Copy link

jovere commented Mar 7, 2023

I'm working on a project where this would be an extremely useful addition. When is this going to get merged?

@DaAwesomeP
Copy link

@stephane what are we missing here?

@kb3ien
Copy link

kb3ien commented Jul 11, 2023

@stephane Indeed I'd like to see this happen too! Can I help test anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants